Skip to content

fix(deps): declare cooklang's bundled_units feature explicitly#371

Merged
dubadub merged 1 commit into
mainfrom
fix/declare-bundled-units
Jul 14, 2026
Merged

fix(deps): declare cooklang's bundled_units feature explicitly#371
dubadub merged 1 commit into
mainfrom
fix/declare-bundled-units

Conversation

@dubadub

@dubadub dubadub commented Jul 14, 2026

Copy link
Copy Markdown
Member

Split out of #369 so it can land on its own — it fixes a latent correctness issue that exists on main right now, independent of any refactoring.

The bug

bundled_units is the cooklang feature carrying the unit database — it is what makes quantities scalable and normalises units. cook recipe depends on it.

cookcli never declared it. Our manifest says:

cooklang = { default-features = false, features = ["aisle", "pantry", "shopping_list"] }

The feature has been arriving purely through Cargo feature unification: cooklang-language-server and cooklang-reports both depend on cooklang with default features, and cooklang's defaults include bundled_units. We have been getting it by accident.

That means any dependency change which drops those crates from the graph silently changes recipe output:

   "quantity": {
-    "scalable": true,
-    "unit": "cups",
+    "scalable": false,
+    "unit": "c",

Nothing about that is obvious from our manifest, and nothing warns you. I only found it because gating lsp off in #369 dropped cooklang-language-server and the snapshot tests went red.

The fix

Declare bundled_units in our own cooklang dependency, where it is actually used.

Verification — this is a no-op today

The point of the change is that it is invisible now and load-bearing later. Confirmed on aarch64:

resolved cooklang features crates
main aisle, bundled_units, default, pantry, prettyplease, quote, shopping_list, syn, sync, toml, toml_edit 412
this PR identical 412
  • cargo test — 13 suites pass, snapshots unchanged (recipe output byte-identical)
  • No new dependencies, no size change

Needs #370 to land first for CI to go green (clippy is broken on main by the Rust 1.97 toolchain bump).

Refs #366

@dubadub dubadub mentioned this pull request Jul 14, 2026
Merged
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review

Small, well-scoped fix — reviewed the diff (Cargo.toml, +5/-1) and the PR description together.

Code quality / correctness

  • The fix is correct: declaring bundled_units explicitly on the cooklang dependency removes an implicit reliance on Cargo feature unification from cooklang-language-server / cooklang-reports. That's a real latent bug — if either of those deps ever drops its default features (or gets removed, as happened in feat: put server, import and lsp behind cargo features #369 when gating lsp off), cook recipe output would silently change (scalable: true/"cups" → scalable: false/"c") with no compile-time signal.
  • The added comment is genuinely earning its place here (explains a non-obvious "why" — the feature-unification hazard — rather than restating the diff), consistent with the repo's comment guidance.
  • Cargo.lock is unchanged, which is expected and correct: the resolved feature set is identical today (verified in the PR description — same 412 crates before/after), so this is a no-op now and a safety net later.

Bugs / security / performance

  • None spotted. This is additive and behavior-preserving by construction; no new dependencies, no size/perf impact.

Test coverage

  • No new test was added, but none is really needed for a manifest-only change like this — the existing snapshot tests (mentioned in the PR body: scalable/unit fields) already act as a regression check for bundled_units being present, and that's how the author caught the original issue in feat: put server, import and lsp behind cargo features #369.
  • One thought for a possible follow-up (not blocking): since this whole class of bug is about implicit feature reliance, it might be worth an occasional cargo tree -e features -p cooklang check (e.g. in CI or doctor) to catch future accidental feature drift across the other declared dependencies too. Not necessary for this PR.

Process note

Overall: looks good to merge once #370 lands and CI is green, per the stated dependency in the PR description.

@dubadub
fix(deps): declare cooklang's bundled_units feature explicitly
e803544 
bundled_units carries the unit database behind quantity scaling and unit
normalisation. cook recipe depends on it, but cookcli never declared it:
it was reaching us purely through cargo feature unification, because
cooklang-language-server and cooklang-reports depend on cooklang with
default features.

Any dependency change that dropped those crates from the graph would
silently alter recipe output - units stop normalising ('cups' instead of
'c') and quantities stop being scalable. Declare it where it is used.

No-op today: resolved cooklang features and crate count are identical.

Refs #366
@dubadub dubadub force-pushed the fix/declare-bundled-units branch from 88d947a to e803544 Compare July 14, 2026 10:46
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review

Small, well-scoped change — reviewed the diff and PR description.

Summary: Cargo.toml now explicitly declares bundled_units in cookcli's own cooklang dependency instead of relying on it arriving implicitly via feature unification from cooklang-language-server/cooklang-reports.

Code quality

  • Correct fix for a real latent bug: features are unified across the whole dependency graph, so depending on bundled_units "for free" via a sibling crate's defaults is fragile — any future change that drops those crates from the graph (exactly what feat: put server, import and lsp behind cargo features #369's optional-features refactor does) would silently flip scalable/unit-normalization behavior with no compile-time signal. Declaring the feature where it's actually used is the right fix.
  • The comment above the dependency line is a good example of "why, not what" — it explains the non-obvious hidden coupling (feature unification) rather than restating the TOML, consistent with this repo's commenting conventions.
  • Good PR hygiene splitting this out from feat: put server, import and lsp behind cargo features #369 as an independently-landable, self-contained fix rather than bundling it into the larger refactor.

Bugs / correctness

None spotted. The change is additive-only (one feature flag), and the PR body includes solid before/after evidence (identical resolved feature set and crate count on main vs. this branch) showing it's a genuine no-op today — which is exactly what you want from a "make an implicit dependency explicit" fix.

Performance

No impact — no new crates pulled in, no size/build-time change, as already confirmed in the PR description.

Security

No concerns — internal Cargo feature-flag change only, no new dependencies or attack surface.

Test coverage

There's no new automated test that pins the resolved cooklang feature set, so the guarantee here currently rests on the existing snapshot suite (which does exercise scaling/unit output) plus the manual cargo tree-style verification in the PR description. That's reasonable for a one-line no-op fix, but it does mean if bundled_units ever regresses again (e.g. if #369 lands and removes the free unification and someone touches this line later), it would only be caught by snapshot diffs rather than an explicit assertion. Not blocking — just worth keeping in mind when #369 lands, since that's the change that actually makes this fix load-bearing; might be worth a quick sanity check with cargo tree -e features -p cooklang (or equivalent) after that merge to confirm bundled_units is still present.

Overall: LGTM, nice catch and a clean, minimal fix.

@dubadub dubadub merged commit cb00cc9 into main Jul 14, 2026
6 checks passed
@dubadub dubadub deleted the fix/declare-bundled-units branch July 14, 2026 11:08
@github-actions github-actions Bot mentioned this pull request Jul 14, 2026
Open
This was referenced Jul 14, 2026
Merged
Merged
dubadub added a commit that referenced this pull request Jul 14, 2026
@dubadub
chore(deps): bump cooklang-language-server to 0.2.4 (#373)
272e573 
0.2.4 stops enabling cooklang's bundled_units feature, which it never
used. Before #371 that would have silently changed cook recipe output,
because we were relying on the language server to turn bundled_units on
for us via feature unification. We now declare it ourselves, so this bump
is a no-op for recipe output - snapshots are unchanged.

Refs #366
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@dubadub